Expand method in import-notation#288
Conversation
Vittly
force-pushed
the
refactor-import-notation
branch
2 times, most recently
from
25d60fb to
16f3739
Compare
Vittly
force-pushed
the
refactor-import-notation
branch
from
16f3739 to
550bbbf
Compare
| Example: | ||
|
|
||
| ```js | ||
| var notation = parse('e:text', { block: 'button' }); |
There was a problem hiding this comment.
Also, I didn't understand the difference from parse method.
Actually, I understood but it was not so obvious
add examples of the distinction between them, please
There was a problem hiding this comment.
Are you sure we need 2 methods? ;-)
There was a problem hiding this comment.
Sure! expand gives us string and it is fast/lightweight
There was a problem hiding this comment.
Are you sure expand is the most viable name for this functionality?
I'd suggest:
fullfillfillupsate
| @@ -1,23 +1,7 @@ | |||
| const hashSet = require('hash-set'); | |||
| const helpers = require('./lib/helpers'); | |||
| main.block = ctx.block; | ||
| main.elem || ctx.elem && (main.elem = ctx.elem); | ||
| } | ||
| switch(type) { |
|
|
||
| describe('elem', () => { | ||
| it('should copy elem if notation has no elem', () => { | ||
| expect(e('m:theme=default', { block : 'button', elem : 'icon' })).to.eql('b:button e:icon m:theme=default'); |
There was a problem hiding this comment.
I think we need a test for block_mod too
('m:theme=default', { block : 'button' })
| describe('context is block', () => { | ||
| it('should extract blockMod', () => { | ||
| expect(p('m:autoclosable', { block : 'popup' })).to.eql([ | ||
| { block : 'popup' }, |
There was a problem hiding this comment.
it was here on purpose, so it's not a patch
but since we have a problem with natural dependency in webpack-bem-loader/babel-plugin-bem-import: bem/webpack-bem-loader#64
maybe we should fix it here.
Need a discussion.
And maybe separate pr. Is it possible @Vittly ?
There was a problem hiding this comment.
hmm, okey I'll do it after adding expand
| modVals = splitMod[1]; | ||
| case 'e': | ||
| cell.elem = tail; | ||
| break; |
There was a problem hiding this comment.
can you add more details?
There was a problem hiding this comment.
Yeah, sorry. we mutating cell but not adding it to acc. For the rest types we call acc.add one or more times.
| /** | ||
| * Helpers to test import-notation first part | ||
| */ | ||
| const tests = { |
There was a problem hiding this comment.
Not sure about this object. Prob it's enough to make 2 simple functions
There was a problem hiding this comment.
I did it a group of functions like templates object after
| main.elem || ctx.elem && (main.elem = ctx.elem); | ||
| } | ||
| switch(type) { | ||
| case 'b': |
There was a problem hiding this comment.
Тоже не любишь делать доп отступ для case? ;-)
There was a problem hiding this comment.
Vasiliy не любит =)
There was a problem hiding this comment.
actually, it's O2 team code style.
|
closed in favor of #312 |
3 participants
Fixing #275.
Also fixed part about "exclude" logic of ctx in parse method (don't know am I right or not). Excluding makes sense only if block requires its modifiers. But I have no real example when it is done. Usually we require modifiers from other blocks and than we need a block too.